update tricrypto-ng #23
Conversation
…revamping claiming fees logic;
…hen pool is not ramping; add comments on how claiming admin fees involves gulping and should be used carefully when optimistic transfers are involved
…we dont gulp anymore
| """ | ||
| received_amounts: uint256 = 0 | ||
| coin_balance: uint256 = ERC20(coins[_coin_idx]).balanceOf(self) | ||
| recorded_balance: uint256 = self.balances[_coin_idx] |
There was a problem hiding this comment.
recorded_balance should go inside if because else is not using it, and we are reading it twice needlessly
| @@ -536,7 +464,7 @@ def add_liquidity( | |||
|
|
|||
| # --------------------- Get prices, balances ----------------------------- | |||
|
|
|||
| precisions: uint256[N_COINS] = self._unpack(self.packed_precisions) | |||
| precisions: uint256[N_COINS] = self._unpack(packed_precisions) | |||
There was a problem hiding this comment.
Why unpack packed precisions every time?? Isn't it better to store them in an immutable array? Then you wouldn't spend gas on unpacking
| # Only enabled in exchange_received: it expects the caller | ||
| # of exchange_received to have sent tokens to the pool before | ||
| # calling this method. | ||
| received_amounts = coin_balance - recorded_balance |
There was a problem hiding this comment.
So here we need received_amounts = coin_balance - self.balances[coin_idx]
There was a problem hiding this comment.
well we need the recorded balances before updating balances. so this won't do.
| fee_params: uint256[3] = self._unpack(self.packed_fee_params) | ||
| f: uint256 = MATH.reduction_coefficient(xp, fee_params[2]) | ||
|
|
||
| # During parameter ramping, we raise fees and disable admin fee claiming |
There was a problem hiding this comment.
Honestly - I don't like raising fees here. I understand it is for safety, but price oracles would be somewhat imprecise when this is the case
| ) | ||
|
|
||
| last_xcp: uint256 = self.get_xcp(self.D, self.price_scale_packed) | ||
| return (last_xcp * (10**18 - alpha) + cached_xcp_oracle * alpha) / 10**18 |
There was a problem hiding this comment.
No, no, this is wrong!! You do like we are doing with price oracles now:
output = self.last_xcp * (1 - alpha) + previous_oracle * alpha
self.last_xcp = self.get_xcp(...)
Otherwise your value can jump - same problem we have with an external oracle now. Yes you'd need to save two variables
…and it seems to be a phishing attack vector
Following compiler/infra related vulnerabilities and exploits, There is now an even more need to footgun-proof the tricrypto-ng contracts.
The existing implementations do not suffer from the vulnerabilities in the following Vyper releases:
https://twitter.com/vyperlang/status/1685692973051498497?s=20
However, native token transfers on EVM increase the chances of introducing unwanted footguns. For instance, the pools vulnerable to malfunctioning reentrancy checks were only exploitable if pool methods had native eth transfers to exploiter.
This pull request attempts to remove the most obvious footguns pre-emptively. Following merge, there will be a proposal to replace the existing tricrypto-ng blueprint contract with the new version.